Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download candidate builds from candidates/ and not nightly/ #221

Closed
wants to merge 1 commit into from
Closed

Download candidate builds from candidates/ and not nightly/ #221

wants to merge 1 commit into from

Conversation

jayrajput
Copy link
Contributor

Updated nightly with candidates in scrapper.py (ReleaseCandidateScrapper::candidate_build_list_regex). Updates to tests.

@whimboo
Copy link
Contributor

whimboo commented Jun 11, 2014

@jayrajput thanks for your patch! Can I ask you to split this PR into two different ones? Each PR should only fix a given issue, but should not cover multiple ones.

@@ -0,0 +1,2 @@
20131001030204
http://hg.mozilla.org/comm-central/rev/6b92cb377496
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you have used git mv for all those changes. If not please reconsider that when you re-create the patch. If you don't do this we will loose all the version history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed #22 from the repository. So now the repository has changes only for #218. I am sending a pull request.

I did not got your comment for "git mv". These are new files added to the repository and not moved. I did copied them from the nightly using unix "cp -R" command and then using git add followed by git commit. What is that you want me to change in respect to #218.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see in my last comment, you cannot copy them all. We only need the candidate builds, but not all the nightly builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when you move the files from nightly/ to candidate/, you should use git mv instead of the plain mv command. See http://githowto.com/moving_files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Henrik,

I have updated the code. I will wait for you to accept the pull request for
#218. I am hoping that I can fix #22 and any other, only when the #218 is
accepted.

Thanks,
Jay

@whimboo whimboo changed the title Fix for #22 and #218 Download candidate builds from candidates/ and not nightly/ Jun 13, 2014
@@ -13,111 +13,111 @@
import mozhttpd_base_test as mhttpd

firefox_tests = [
# -p win32 -v 21.0
# -p win32 -v 28.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of our tests, we have those to verify that download will succeed. We are aware of that test data will not always be up-to-date. Means we don't want to update the version numbers each time we test those files. It doesn't matter for us, if it is Firefox 10.0, 21.0, or 28.0. Important is that we download the correct version. Only in case that the structure on the FTP server is changing, we will have to make sure to update those versions. So please only move the test files to the new location, and update the target_url in terms of the nightly -> candidates change.

@jayrajput
Copy link
Contributor Author

made changes as suggested. Also ran the run_tests.py and it passed. I missed it during commit of 61f3555 (#218: merge changes). Need to learn to reduce my number of commits. Still learning git and all the new tools. Thanks.

@whimboo
Copy link
Contributor

whimboo commented Jun 18, 2014

Thanks Jay for the update. This looks lovely now. Would you mind to rebase all the commits into a single one? Once done I can land your PR.

@jayrajput
Copy link
Contributor Author

I have rebased all the commits as suggested. The changes can be merged now. Let me know if you have any more comments.

@whimboo
Copy link
Contributor

whimboo commented Jun 25, 2014

I fixed the commit message to align with our specs, and merged the PR as d4eba84.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants